Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏗️✨ Add task to verify package manifests #11

Merged
merged 6 commits into from
Aug 2, 2022

Conversation

DerekNonGeneric
Copy link
Owner

@DerekNonGeneric DerekNonGeneric commented Mar 14, 2020

With package.json files taking center-stage in several ES module features (not exclusive to packages), and the emergence of npm workspaces, I'd like to ensure consistency and validity amongst them. This is still a WIP, but it currently works pretty well. I had been using it in my monorepo setup, but was able to cleanly separate it. I credit @ljharb for much of the idea as it began as an attempt to solve a problem he brought up on Slack. Hopefully, folks will find this useful and we'd avoid duplicating our efforts here.

I've been thinking a bit about the scope of this project and I think one appropriate non-goal would be to depend on engines beyond the main engines of the Node.js toolchain (node, npm, npx). The thought came to mind when I was thinking about how workspaces and self-referential modules would be interacting together in the same project. I haven't yet read the RFC above, but that would be next.

@DerekNonGeneric DerekNonGeneric force-pushed the master branch 3 times, most recently from 0ee6d76 to 409b354 Compare March 14, 2020 14:56
@DerekNonGeneric DerekNonGeneric force-pushed the feat/verify-manifests branch 3 times, most recently from b88fb87 to 074e092 Compare March 14, 2020 15:26
@DerekNonGeneric DerekNonGeneric force-pushed the feat/verify-manifests branch 8 times, most recently from 2867f5d to cc5fc0b Compare March 15, 2020 09:05
@DerekNonGeneric DerekNonGeneric force-pushed the master branch 12 times, most recently from 07351a7 to c58ee73 Compare March 19, 2020 00:06
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must this be a loader? or can it be used as an eslint plugin, or a standalone CLI?

Comment on lines +4 to +5
* Much of the order of these keys follow the order of appearance seen in
* npm's `man` page. Additionally found [here].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this verified by CI? seems doable and also would ensure no unknown changes happen

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not currently, what CI do you want to use?

  • Travis
  • GitHub Actions
  • Circle
  • Something else?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the least, by npm test - personally i use travis everywhere, but you could use Github Actions too.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GitHub outages are a bit unnerving, but I'm willing to do it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb, for some reason I got the impression that you felt that all of the package.json keys were supposed to be in alphabetical order, but that's not what this PR is doing. I would like to get to the bottom of this… What is the order we should use? lol

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@DerekNonGeneric DerekNonGeneric marked this pull request as ready for review August 2, 2022 22:53
@DerekNonGeneric
Copy link
Owner Author

I need to keep iterating on this… I guess I will have to keep you posted and let you know once I think I finally have it ideal @ljharb. I still want to ensure CI can check this, but I plan to get all this config published and sharable between projects as dependencies to alleviate the huge mess it makes. Once that is done, I will get it checked for by GitHub Actions. Thanks for the tips!

@DerekNonGeneric DerekNonGeneric merged commit 18d25eb into main Aug 2, 2022
@DerekNonGeneric DerekNonGeneric deleted the feat/verify-manifests branch August 3, 2022 23:40
@DerekNonGeneric DerekNonGeneric added the dependencies Pull requests that update a dependency file label Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants